[hdrhistogram]: Prevent an FPE in rd_hdr_histogram_new (#4949) #5197
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
Neofish22
force-pushed
the
bugfix/4949-fpe-in-rd_hdr_histogram_new
branch
from
be57f18 to
50f3850
Compare
|
Was unable to get trivup to work correctly, will try again as soon as I can (most likely next week; business trip). |
Neofish22
force-pushed
the
bugfix/4949-fpe-in-rd_hdr_histogram_new
branch
from
3ef2658 to
b9a1072
Compare
…4949) The Floating Point Exception (FPE) `FE_DIVBYZERO` is flagged in `rd_hdr_histogram_new` when `log2()` is called with `0`. This is done in `rd_avg_init` numerous times, some of which cannot be disabled in recent versions. Consumers of librdkakfa may enable the raising of exceptions when FPEs occur, including `FE_DIVBYZERO`, which causes the function to fail. It is reasonable for the callers of `rd_hdr_histogram_new` to pass `0` for the `minValue` parameter, and the intention of the calculation of `unitMagnitude` is also reasonable. As such, fixing the maths there is the correct approach. `unitMagnitude` being `0` in this case is an apt choice based on how log2(n) works for n near `0`, and that the caluclation already minimises to `0`. A simple test was included that resets the flags, runs the offending function, and observes the new set flags. Appropriately, it fails before the fix, and passes after.
|
Have not had time to try to get trivup and the test suite to work properly, but I have not forgotten this. 🙈 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit message:
Description
First time doing a Pull Request on an open source project so please bear with me. :) Fixes #4949
I believe I have followed the contributing guidelines correctly; I have:
make style-check-changedon the source file I changed and the source file I added.CHANGELOG.md.make release-test. (TODO - I need to install trivup for that).Potential issue in build config
One potential issue is that I had to modify the build process to link
test-runnerwith-lrdkafka-staticbecause it did not find the definition forrd_hdr_histogram_new. I assume this is something wrong with my build configuration, but if not we will need to do something to rectify it.